Better ingress error message on permission denied #1666
Conversation
google-cla
bot
added
the
cla: yes
pkg/broker/ingress/handler.go
Outdated
| case grpcstatus.Code(res) == grpccode.PermissionDenied: | ||
| const msg string = "Failed to publish to PubSub because permission denied.\n" + | ||
| "Please refer to \"Configure the Authentication Mechanism for GCP\" at " + | ||
| "https://github.com/google/knative-gcp/blob/master/docs/install/install-knative-gcp.md" |
There was a problem hiding this comment.
It might be better to point broker installation page(https://github.com/google/knative-gcp/blob/master/docs/install/install-gcp-broker.md) rather than install knative-gcp page (this is for the Control Plane setup)
There was a problem hiding this comment.
Thanks Grace! Modified accordingly.
cathyzhyi
force-pushed
the
ingress-error
branch
from
244d3c1
to
8203b84
Compare
|
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-google-knative-gcp-wi-tests: |
|
/retest |
cathyzhyi
force-pushed
the
ingress-error
branch
from
8203b84
to
ae00d51
Compare
pkg/broker/ingress/handler.go
Outdated
| const msg string = "Failed to publish to PubSub because permission denied.\n" + | ||
| "Please refer to \"Configure the Authentication Mechanism for GCP\" at " + | ||
| "https://github.com/google/knative-gcp/blob/master/docs/install/install-gcp-broker.md" |
There was a problem hiding this comment.
At the top you can define a constant like
const deniedErrMsg string = `Failed to publish to PubSub because permission denied.
Please refer to "Configure the Authentication Mechanism for GCP" at https://github.com/google/knative-gcp/blob/master/docs/install/install-gcp-broker.md
`
And use it here.
There was a problem hiding this comment.
Moved the const string definition to the top of the file as suggested. Thanks!
pkg/broker/ingress/handler.go
Outdated
| statusCode = nethttp.StatusServiceUnavailable | ||
| case grpcstatus.Code(res) == grpccode.PermissionDenied: | ||
| const msg string = "Failed to publish to PubSub because permission denied.\n" + |
There was a problem hiding this comment.
Also add a TODO. I think eventually we want to put a link to the official doc rather than github doc.
There was a problem hiding this comment.
Added TODO. thanks for the suggestion!
|
Since this has impact on user facing function, it's best to add a release note. I helped to add it for this PR. |
cathyzhyi
force-pushed
the
ingress-error
branch
from
ae00d51
to
0752cd7
Compare
cathyzhyi
force-pushed
the
ingress-error
branch
from
0752cd7
to
539e657
Compare
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cathyzhyi, yolocs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #1631
Proposed Changes
Release Note
Docs